Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New OCS endpoint to list text processing tasks #39680

Merged
merged 6 commits into from
Aug 7, 2023
Merged

New OCS endpoint to list text processing tasks #39680

merged 6 commits into from
Aug 7, 2023

Conversation

julien-nc
Copy link
Member

@julien-nc julien-nc commented Aug 2, 2023 •
edited
Loading

refs nextcloud/text#4613

This adds a /tasks/app/{appId} endpoint to get a list of tasks for the current user, for a specific app and optionally with a specific identifier.

While playing with the API, I discovered that it's impossible to schedule a task while not being authenticated (mostly because of a NotNull Db constraint on the user_id column). But all the endpoints are public. Maybe we should sort this out first.

  • Do we want to allow non-authenticated users to schedule a job?
    • If so, do we want to be able to list all tasks that were scheduled publicly?
    • If not, should we require auth for all those endpoint?
  • Are we fine with a user only being able to list his/her own tasks?
    • This would mean publicly scheduled tasks can be listed by any other non-authenticated user. Are we fine with that?

TODO

  • add proper index(es)
  • allow authenticated users to list all their tasks by app (and optionally by identifier)

@julien-nc julien-nc added this to the Nextcloud 28 milestone Aug 2, 2023
@julien-nc julien-nc force-pushed the enh/tp-list-tasks-by-app-and-identifier branch 2 times, most recently from 2291fd1 to 8ee178b Compare August 3, 2023 21:24
@julien-nc
Copy link
Member Author

What's been done:

  • Convert annotations to Php method attributes
  • Rename languageModelManager to textProcessingManager in the API controller
  • Add endpoint for authenticated users to list their tasks by appId (and optionally by identifier)
  • Add manager and mapper methods to:
    • get a task by user id and task id
    • get a list of task by user id and app id (and optionally with a specific identifier)
  • Allow null user_id in textprocessing_tasks so tasks can be scheduled anonymously
  • Add user_id, app_id, identifier index to textprocessing_tasks

@juliushaertl So it's only possible to list tasks by app while being authenticated. This is to prevent anonymous users to be able to list all tasks that were created by other anonymous users. Any problem with that in the Text integration?

@julien-nc julien-nc marked this pull request as ready for review August 3, 2023 21:26
@juliusknorr
Copy link
Member

So it's only possible to list tasks by app while being authenticated. This is to prevent anonymous users to be able to list all tasks that were created by other anonymous users. Any problem with that in the Text integration?

Fine with me as the MVP. I'd then limit the text integration to logged in users only for now.

@marcelklehr marcelklehr added the pending documentation This pull request needs an associated documentation update label Aug 4, 2023
@juliusknorr juliusknorr mentioned this pull request Aug 4, 2023
Merged
13 tasks
@marcelklehr marcelklehr mentioned this pull request Aug 7, 2023
Closed
5 tasks
julien-nc and others added 5 commits August 7, 2023 13:27
@julien-nc
start implementing ocs endpoint to get task list from user+appId+iden…
9986e02 
…tifier

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
allow anon text processing scheduling
41b19cf 
add a textprocessing_tasks index
convert anotations to method attributes
refactor TP manager
add mapper methods

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
change sql where order to match index order
05a6a79 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@juliusknorr @julien-nc
feat: Add delete task API
fca1c30 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@julien-nc
bump server version to 28.0.0.2 to trigger new migration step
e9e7db9 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc force-pushed the enh/tp-list-tasks-by-app-and-identifier branch from b8321ef to e9e7db9 Compare August 7, 2023 11:29
@julien-nc
Copy link
Member Author

Rebased on master, bumped server version to 28.0.0.2 to trigger the migration step. All good IMO.

@julien-nc julien-nc requested a review from kesselb August 7, 2023 11:30
juliusknorr
juliusknorr approved these changes Aug 7, 2023
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good, tested and works for my use case. 👍

Psalm is still not happy though.

marcelklehr
marcelklehr approved these changes Aug 7, 2023
View reviewed changes
Copy link
Member

@marcelklehr marcelklehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨

please make sure to fix psalm, though

@julien-nc
fix psalm issue
f154fe7 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc force-pushed the enh/tp-list-tasks-by-app-and-identifier branch from 2609d8f to f154fe7 Compare August 7, 2023 16:29
@juliusknorr
Copy link
Member

Failing drone acceptance test is unrelated, also there on master https://drone.nextcloud.com/nextcloud/server/38240/57/4

@juliusknorr juliusknorr merged commit eb2f3bc into master Aug 7, 2023
37 checks passed
@juliusknorr juliusknorr deleted the enh/tp-list-tasks-by-app-and-identifier branch August 7, 2023 19:50
@juliusknorr
Copy link
Member

@julien-nc @marcelklehr Is there already documentation for the text processing to extend on?

@juliusknorr
Copy link
Member

Not triggering the backport yet, but I have it on my list of requirements for 27.1

@marcelklehr
Copy link
Member

@juliushaertl

Yes, text processing docs have already been merged, should be easy to extend for this :)

I've also created #39738 :)

@marcelklehr marcelklehr mentioned this pull request Aug 9, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@marcelklehr marcelklehr marcelklehr approved these changes

@kesselb kesselb Awaiting requested review from kesselb

Assignees
No one assigned
Labels
2. developing Work in progress backport-request enhancement integration pending documentation This pull request needs an associated documentation update
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.
4 participants
@julien-nc @juliusknorr @marcelklehr @kesselb